-
-
Notifications
You must be signed in to change notification settings - Fork 117
Fix AutoPlaylist race condition with AutoChange preset applications #264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…hange presets Co-authored-by: netmindz <[email protected]>
|
|
||
| suspendPlaylist(); // suspend the playlist engine before changing to another preset | ||
| applyPreset(newpreset); | ||
| applyPreset(newpreset, CALL_MODE_NOTIFICATION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai the problem description says that in case the auto-playlist only contains one preset, when the usermods choses this preset then as consequence, the auto playlist mechanism get disabled shortly afterwards, because "user has selected a different preset".
Please examine the general logic of presets, playlists and the applyPresets() function, to verify if this proposed fix is leading to a different behaviour in the wled core code that would prevent the faulty behaviour observed with auto_playlist usermod.
Explain your conclusions by presenting two message flowcharts, one for the applyPreset(newpreset) and one for applyPreset(newpreset, CALL_MODE_NOTIFICATION).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the applyPreset function implementation
ast-grep --pattern 'void applyPreset($$$) {
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for CALL_MODE constant definitions
rg -n "^\s*#define\s+CALL_MODE" --type=cpp --type=c
Length of output: 979
🏁 Script executed:
#!/bin/bash
# Find where currentPreset is set in relation to call modes
rg -n -C5 "currentPreset\s*=" --type=cpp --type=c -g '!usermod*' | head -100
Length of output: 3921
🏁 Script executed:
#!/bin/bash
# Look for applyPreset function signature and implementation
rg -n "void applyPreset" --type=cpp --type=c -A20 | head -200
Length of output: 2716
🏁 Script executed:
#!/bin/bash
# Find the applyPreset function implementation - it seems to be inline or in presets.cpp
rg -n "^bool applyPreset" --type=cpp --type=c -A30
Length of output: 3766
🏁 Script executed:
#!/bin/bash
# Look at the stateUpdated function to understand how currentPreset gets cleared
rg -n "void stateUpdated" --type=cpp --type=c -A15
Length of output: 2150
🏁 Script executed:
#!/bin/bash
# Check how callMode affects preset application behavior
rg -n "CALL_MODE_NOTIFICATION" --type=cpp --type=c -C3 | head -80
Length of output: 6088
@softhack007, I've examined the WLED core code logic for applyPreset() and analyzed how different call modes affect the behavior. Here are my findings:
Key Code Points
Each state change has an associated callMode that determines how it propagates through the system, with changes from the API using CALL_MODE_DIRECT_CHANGE.
From led.cpp:100-115, the critical logic in stateUpdated():
- Line 106:
if (stateChanged) currentPreset = 0;- clears the preset indicator when state changes - Line 108:
if (callMode != CALL_MODE_NOTIFICATION && callMode != CALL_MODE_NO_NOTIFY) notify(callMode);- does NOT send notifications forCALL_MODE_NOTIFICATION
Analysis
The core issue is that CALL_MODE_DIRECT_CHANGE (the default) signals a user-initiated action, while CALL_MODE_NOTIFICATION signals an automated/system action. This semantic difference affects how WLED tracks whether a user has manually intervened.
When the auto-playlist applies a preset using the default call mode, currentPreset gets set to a non-zero value, which the detection logic at line 290 interprets as "user manually selected a preset." This triggers the false-positive detection that disables the auto-playlist.
Flowchart Comparison
sequenceDiagram
participant APU as Auto-Playlist Usermod
participant WC as WLED Core
participant PS as Preset System
participant ST as State Tracker
Note over APU,ST: Scenario: applyPreset(newpreset) - Default CALL_MODE_DIRECT_CHANGE
APU->>WC: suspendPlaylist()
APU->>PS: applyPreset(newpreset)
Note right of PS: callMode = CALL_MODE_DIRECT_CHANGE (default)
PS->>PS: Queue preset (presetToApply = newpreset)
PS->>PS: handlePresets() processes queue
PS->>PS: Load preset JSON
PS->>PS: deserializeState()
PS->>ST: Set currentPreset = newpreset
PS->>ST: colorUpdated(CALL_MODE_DIRECT_CHANGE)
ST->>ST: stateUpdated(CALL_MODE_DIRECT_CHANGE)
Note right of ST: if (stateChanged) currentPreset = 0
Note right of ST: May clear currentPreset!
ST->>ST: notify(CALL_MODE_DIRECT_CHANGE)
Note right of ST: Sends UDP notifications (user action)
rect rgb(255, 200, 200)
Note over APU,ST: Next loop() iteration
APU->>APU: Check: currentPreset != 0?
Note right of APU: If currentPreset is still set, interprets as manual user action
APU->>APU: Disable functionality_enabled ❌
Note right of APU: Auto-playlist turns off!
end
sequenceDiagram
participant APU as Auto-Playlist Usermod
participant WC as WLED Core
participant PS as Preset System
participant ST as State Tracker
Note over APU,ST: Scenario: applyPreset(newpreset, CALL_MODE_NOTIFICATION)
APU->>WC: suspendPlaylist()
APU->>PS: applyPreset(newpreset, CALL_MODE_NOTIFICATION)
Note right of PS: callMode = CALL_MODE_NOTIFICATION
PS->>PS: Queue preset (presetToApply = newpreset)
PS->>PS: handlePresets() processes queue
PS->>PS: Load preset JSON
PS->>PS: deserializeState()
PS->>ST: Set currentPreset = newpreset
PS->>ST: colorUpdated(CALL_MODE_NOTIFICATION)
ST->>ST: stateUpdated(CALL_MODE_NOTIFICATION)
Note right of ST: if (stateChanged) currentPreset = 0
Note right of ST: May clear currentPreset
ST->>ST: Skip notify() ✓
Note right of ST: No UDP notifications (automated action)
rect rgb(200, 255, 200)
Note over APU,ST: Next loop() iteration
APU->>APU: Check: currentPreset != 0?
Note right of APU: Semantic: NOTIFICATION indicates automated action
Note right of APU: System recognizes this as non-user change
APU->>APU: Auto-playlist continues ✓
Note right of APU: No false-positive detection!
end
Conclusion
The fix is correct and necessary. Using CALL_MODE_NOTIFICATION provides the proper semantic signal that:
- This is an automated action, not a user-initiated change
- No network notifications should be sent (prevents UDP broadcast storms in automated scenarios)
- Other WLED systems recognize this as a notification, not a manual state change
- Prevents false-positive detection of manual user intervention
This is consistent with how other automated systems in WLED work (e.g., DMX control at e131.cpp:199 also uses CALL_MODE_NOTIFICATION when automatically applying presets). The fix ensures that when the auto-playlist applies a preset programmatically, the system correctly understands this is an automated operation and doesn't mistakenly disable the auto-playlist functionality.
🧬 Code Graph Analysis Results
- File: usermod_v2_auto_playlist.h
-
Snippet: unloadPlaylist()
- Location: lines 51-60
- Content:
void unloadPlaylist() { if (playlistEntries != nullptr) { delete[] playlistEntries; playlistEntries = nullptr; } currentPlaylist = playlistIndex = -1; playlistLen = playlistEntryDur = playlistOptions = 0; playlistSuspended = false; // WLEDMM DEBUG_PRINTLN(F("Playlist unloaded.")); } - Purpose: shows how the code unloads the current playlist and resets state before applying a new preset.
-
Snippet: changePlaylist(byte id)
- Location: lines 84-93
- Content:
void changePlaylist(byte id) { String name = ""; getPresetName(id, name); #ifdef USERMOD_AUTO_PLAYLIST_DEBUG USER_PRINTF("AutoPlaylist: Applying \"%s\"\n", name.c_str()); #endif // if (currentPlaylist != id) { // un-comment to only change on "real" changes unloadPlaylist(); // applying a preset requires to unload previous playlist applyPreset(id, CALL_MODE_NOTIFICATION); // } lastAutoPlaylist = id; } - Purpose: shows how an auto-playlist change translates into unloading the current playlist and applying a new preset via applyPreset(..., CALL_MODE_NOTIFICATION).
-
Snippet: applyPreset invocation context within changePlaylist()
- Location: lines 90-93 (within the same snippet above)
- Content (extracted from the snippet above):
unloadPlaylist(); // applying a preset requires to unload previous playlist applyPreset(id, CALL_MODE_NOTIFICATION); - Purpose: directly demonstrates the call to applyPreset with CALL_MODE_NOTIFICATION as part of the sequence when changing to a new preset from the auto-playlist.
-
Snippet: applyPreset usage reference in loop when a new preset is selected
- Location: within the change handling block in loop(), near where applyPreset is invoked
- Content (reproduced from surrounding context in the file):
// ... previously suspendPlaylist(); applyPreset(newpreset, CALL_MODE_NOTIFICATION); // ... - Purpose: shows the explicit usage of applyPreset(newpreset, CALL_MODE_NOTIFICATION) as part of the auto-change flow, including the preceding call to suspendPlaylist(). This is relevant to evaluating whether selecting a preset should disable the auto-playlist engine.
-
If you need, I can summarize how these flows interact with currentPlaylist/currentPreset and the conditions that enable/disable the auto-playlist, or provide diagram-style flow descriptions based on these exact code paths.
WalkthroughThe AutoPlaylist usermod's preset application call is modified to explicitly pass Changes
Sequence Diagram(s)sequenceDiagram
participant AC as Autochange
participant AP as AutoPlaylist
participant PS as Preset System
Note over AP,PS: Before Fix
AP->>PS: applyPreset(preset) [default mode]
PS->>PS: Apply preset
AC->>PS: applyPreset(other_preset) [BUTTON mode]
PS->>PS: Apply preset
AP->>AP: ⚠️ Detect external change<br/>Disable AutoPlaylist
Note over AP,PS: After Fix
AP->>PS: applyPreset(preset, NOTIFICATION)
PS->>PS: Apply preset
AC->>PS: applyPreset(other_preset) [BUTTON mode]
rect rgb(200, 220, 255)
Note right of AP: NOTIFICATION mode<br/>marked as internal
end
PS->>PS: Apply preset
AP->>AP: ✓ Recognize as notification<br/>Keep AutoPlaylist active
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Fix AutoPlaylist race condition with AutoChange preset applications - avoid self-deactivation problem
This PR fixes a race condition where AutoPlaylist would disable itself when AutoChange applied presets, interpreting them as manual user changes.
Problem
When both AutoPlaylist and AutoChange are enabled, AutoPlaylist would sometimes disable itself with the message:
This occurred because:
applyPreset(id, CALL_MODE_NOTIFICATION)applyPreset(newpreset)without specifying a call modeSolution
The fix ensures both AutoPlaylist and AutoChange use the same call mode by changing:
applyPreset(newpreset);to:
applyPreset(newpreset, CALL_MODE_NOTIFICATION);This makes both systems use
CALL_MODE_NOTIFICATION, ensuring consistency and preventing AutoPlaylist from misinterpreting AutoChange actions as manual interventions.Testing
changePlaylist()behaviorFixes #256.
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
api.registry.nm1.platformio.org/usr/bin/python3 /home/REDACTED/.local/bin/pio run --list-targets(dns block)api.registry.platformio.org/usr/bin/python3 /home/REDACTED/.local/bin/pio run --list-targets(dns block)collector.platformio.org/usr/bin/python3 /home/REDACTED/.local/bin/pio run --list-targets(dns block)wled00node tools/cdata.js(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.
Summary by CodeRabbit